Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION #11

ludanpr · 2021-02-21T20:19:32Z

DEFINE-CONDITION seems to have an issue. When called with an object that is not a
CONDITION's subtype, it accepts it anyway:

CL-USER> (defclass any-class () ())
;; #<STANDARD-CLASS COMMON-LISP-USER::ANY-CLASS>
CL-USER> (macroexpand-1 '(pcs:define-condition foo-condition (any-class other-class)
                          ((foo-slot :reader foo-condition-slot :initarg :slot))
                          (:report "A Report")))

;; (PROGN
;;  (DEFCLASS FOO-CONDITION (ANY-CLASS OTHER-CLASS)
;;            ((FOO-SLOT :READER FOO-CONDITION-SLOT :INITARG :SLOT)))
;;  (DEFMETHOD PRINT-OBJECT ((#:CONDITION759 FOO-CONDITION) #:STREAM760)
;;    (WRITE-STRING "A Report" #:STREAM760))
;;  'FOO-CONDITION)
;; T
CL-USER> (pcs:define-condition foo-condition (any-class other-class)
          ((foo-slot :reader foo-condition-slot :initarg :slot))
          (:report "A Report"))

;; FOO-CONDITION
CL-USER> (subtypep 'foo-condition 'any-class)
;; T
;; T
CL-USER> (subtypep 'foo-condition 'pcs:condition)
;; NIL
;; NIL

Adds COERCE-CONDITION-SUPERTYPES and changes EXPAND-DEFINE-CONDITION.
Add DEFINE-CONDITION-INTERNAL to handle internal conditions (DEFINE-CONDITION-INTERNAL
is necessary because we don't know beforehand the relationship between the
internal conditions and CONDITION, needed by COERCE-CONDITION-SUPERTYPES.)

The proposed fix results in:

CL-USER> (defclass any-class () ())
;; #<STANDARD-CLASS COMMON-LISP-USER::ANY-CLASS>
CL-USER> (macroexpand-1 '(pcs:define-condition foo-condition (any-class other-class)
                          ((foo-slot :reader foo-condition-slot :initarg :slot))
                          (:report "A Report")))
;; (PROGN
;;  (DEFCLASS FOO-CONDITION (PORTABLE-CONDITION-SYSTEM:CONDITION)
;;            ((FOO-SLOT :READER FOO-CONDITION-SLOT :INITARG :SLOT)))
;;  (DEFMETHOD PRINT-OBJECT ((#:CONDITION747 FOO-CONDITION) #:STREAM748)
;;    (WRITE-STRING "A Report" #:STREAM748))
;; 'FOO-CONDITION)
;; T
CL-USER> (pcs:define-condition foo-condition (any-class other-class)
          ((foo-slot :reader foo-condition-slot :initarg :slot))
          (:report "A Report"))
;; FOO-CONDITION
CL-USER> (subtypep 'foo-condition 'any-class)
;; NIL
;; T
CL-USER> (subtypep 'foo-condition 'pcs:condition)
;; T
;; T

I'm using SBCL 2.0.11

DEFINE-CONDITION-INTERNAL to handle internal conditions (DEFINE-CONDITION-INTERNAL is necessary because we don't know beforehand the relationship between the internal conditions and CONDITION, needed by COERCE-CONDITION-SUPERTYPES.)

phoe

Thanks for the patch! I've left two general remarks, plus a request for an addition to the test suite that verifies those scenarios.

src/conditions.lisp

phoe · 2021-02-21T20:30:33Z

src/condition-hierarchy.lisp

@@ -8,13 +8,13 @@
 ;;; documentation strings due to the repetitiveness of the contents of this
 ;;; file.

-(define-condition warning (condition) ())
+(define-condition-internal warning (condition) ())


None of these changes from define-condition to define-condition-internal should be required. Since condition already names a condition type, we should be able to use define-condition instead of define-condition-internal.

Why were they added?

When running the test system with DEFINE-CONDITION here, I get INVALID INITIALIZATION ARGUMENTS errors like:

;; Invalid initialization arguments: ;; :EXPECTED-TYPE, :DATUM ;; in call for class #<STANDARD-CLASS PORTABLE-CONDITION-SYSTEM::CASE-FAILURE>. ;; [Condition of type SB-PCL::INITARG-ERROR] ;; See also: ;; Common Lisp Hyperspec, 7.1.2 [:section]

The way COERCE-CONDITION-SUPERTYPES was defined, it was filtering out PCS:SERIOUS-CONDITION and PCS:ERROR from the PCS::CASE-FAILURE condition, for example. But they were superclasses of PCS::CASE-FAILURE before the changes. I assume SUBTYPEP in COERCE-CONDITION-SUPERTYES couldn't tell the relations at this point?

Yes, this is what I feared - that subtypep, when run at compile-time, does not have information about class definitions. If left as-is, this is going to propagate into users' code, and this may break in weird ways for them where loading a file is going to have different semantics than compiling it.

A possible (and better!) solution I can imagine is adding a check into the class (re)definition process itself - e.g. a :before method on ensure-class-using-class can ensure that all superclasses are condition or proper subclasses of condition.

Yes, this isn't acceptable. Thank you for the feedback.

I'm following your suggestion about ensure-class-using-class, and came across a doubt. What should be the behavior of:

(define-condition foo-condition () ()) (defclass bar (foo-condition) ())

?

sbcl, for example, signals an INVALID-SUPERCLASS

I guess we can either leave this behavior undefined (because people are supposed to unconditionally use define-condition to define condition types), or signal our own error in such a situation.

…a non-CONDITION Other changes: Adds a INVALID-SUPERCLASS condition. Signals INVALID-SUPERCLASS when a DEFCLASS outside of DEFINE-CONDITION tries to superclass a CONDITION.

…a non-CONDITION Merge branch 'issue-define-condition' of github.com:lycankrammer/portable-condition-system into issue-define-condition

phoe

Sorry about the long wait - I did not notice that you have made changes after my initial round of reviews and that you requested another review.

phoe · 2022-02-06T17:36:25Z

src/conditions.lisp

@@ -47,17 +47,44 @@ PRINT-OBJECT method defined on the class named by NAME."
    `(let ((,method (find-method #'print-object '() '(,name t) nil)))
       (when ,method (remove-method #'print-object ,method)))))

+(defvar *in-define-condition-p* nil
+  "This will be dynamically binded to indicate an entry into DEFINE-CONDITION,


s/binded/bound/

phoe · 2022-02-06T17:37:43Z

src/conditions.lisp

+(defmethod ensure-class-using-class :before (class name &rest args &key direct-superclasses &allow-other-keys)
+  "In DEFINE-CONDITION: Signals a TYPE-ERROR at the first direct superclass unconforming
+with DEFINE-CONDITION, that is, the first encountered superclass in DIRECT-SUPERCLASSES
+that is not a subtype of CONDITION.


...subclass of CONDITION

phoe · 2022-02-06T17:39:26Z

src/conditions.lisp

+            :do (when (not (subtypep superclass 'condition))
+                  (error 'type-error
+                         :datum superclass
+                         :expected-type 'condition)))


This error is wrong - such a TYPE-ERROR would mean that the superclass is not of type CONDITION, whereas here the error should say that the superclass is not a subtype of CONDITION. Hence, I think that you should go for signaling INVALID-SUPERCLASS in here too.

phoe · 2022-02-06T17:40:18Z

src/conditions.lisp

+encountered superclass that is subtype of CONDITION."
+  (declare (ignore args))
+  (if *in-define-condition-p*
+      (loop :for superclass :in direct-superclasses


Maybe switch out the LOOPs for DOLISTs? Just a style thing.

phoe · 2022-02-06T17:40:23Z

src/conditions.lisp

+                  (error 'type-error
+                         :datum superclass
+                         :expected-type 'condition)))
+      (loop :for superclass :in direct-superclasses


phoe · 2022-02-06T17:43:59Z

src/condition-hierarchy.lisp

+          (invalid-superclass-superclass condition)
+          (invalid-superclass-reason condition)))
+
+(define-condition invalid-superclass (condition)


That's an obvious ERROR rather than a CONDITION.

phoe · 2022-02-06T17:45:49Z

src/conditions.lisp

+  (if *in-define-condition-p*
+      (loop :for superclass :in direct-superclasses
+            :do (when (not (subtypep superclass 'condition))
+                  (error 'type-error


I am kind of interested in the topic of dogfooding ourselves with this one. We will be using PCS:ERROR to signal PCS:INVALID-SUPERCLASS during class definition, which will mostly only work with Slime if PCS integration is loaded. Nice.

phoe · 2022-02-06T17:47:47Z

src/conditions.lisp

-                  `(,(expand-define-condition-remove-report-method name)))
-            ',name)))
+    `(progn
+       (let ((*in-define-condition-p* t))


This solution creates some new problems. The main issue of having a LET here is that the DEFCLASS is no longer toplevel which means that condition classes are not going to be picked up by the compiler, which can result in undefined type warnings.

Not sure its a good idea, but an alternative I see would be to add a dummy class to supertypes here in EXPAND-DEFINE-CONDITION . Do you have any suggestion?

phoe · 2022-02-06T17:48:57Z

t/ansi-test/define-condition.lisp

@@ -579,6 +579,41 @@
  (notnot-mv (typep #'condition-27/s1 'generic-function))
  t)

+;;; Test non-CONDITION supertypes


Could you move this whole section to more-tests.lisp? I'd rather not modify the ANSI-TEST files themselves.

phoe requested changes Feb 21, 2021

View reviewed changes

ludanpr added 2 commits March 3, 2021 10:04

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype …

9bc7c96

…a non-CONDITION Other changes: Adds a INVALID-SUPERCLASS condition. Signals INVALID-SUPERCLASS when a DEFCLASS outside of DEFINE-CONDITION tries to superclass a CONDITION.

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype …

a1e513d

…a non-CONDITION Merge branch 'issue-define-condition' of github.com:lycankrammer/portable-condition-system into issue-define-condition

ludanpr changed the title ~~Change DEFINE-CONDITION to allow and handle only CONDITION's subtypes~~ Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION Mar 11, 2021

ludanpr requested a review from phoe March 11, 2021 14:50

phoe requested changes Feb 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION #11

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION #11

ludanpr commented Feb 21, 2021

phoe left a comment

phoe Feb 21, 2021

ludanpr Feb 21, 2021

phoe Feb 21, 2021

ludanpr Feb 21, 2021

ludanpr Feb 24, 2021

phoe Mar 2, 2021

phoe left a comment

phoe Feb 6, 2022

phoe Feb 6, 2022

phoe Feb 6, 2022

ludanpr Feb 12, 2022

phoe Feb 6, 2022

ludanpr Feb 12, 2022

phoe Feb 6, 2022

phoe Feb 6, 2022

ludanpr Feb 12, 2022

phoe Feb 6, 2022

phoe Feb 6, 2022

ludanpr Feb 12, 2022

phoe Feb 6, 2022

ludanpr Feb 12, 2022

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION #11

Are you sure you want to change the base?

Change DEFINE-CONDITION: signals a TYPE-ERROR if trying to supertype a non-CONDITION #11

Conversation

ludanpr commented Feb 21, 2021

phoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment